Skip to content

Conversation

@mtfishman
Copy link
Member

This uses task_local_storage in the Index ID RNG to avoid thread-local states as recommended in https://julialang.org/blog/2023/07/PSA-dont-use-threadid.

Issues with the previous design were pointed out by @amilsted.

@mtfishman
Copy link
Member Author

@amilsted can you test this out on cases where the previous code was failing?

@codecov
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.91%. Comparing base (93dc9bd) to head (9eebb9e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1646      +/-   ##
==========================================
- Coverage   80.93%   80.91%   -0.02%     
==========================================
  Files          59       59              
  Lines        4636     4626      -10     
==========================================
- Hits         3752     3743       -9     
+ Misses        884      883       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amilsted
Copy link

We monkey-patched those changes into an older version and it indeed fixed the assertion errors we were seeing.

FWIW, just using the default_rng() also worked. Is it functionally any different than this?

@mtfishman
Copy link
Member Author

We monkey-patched those changes into an older version and it indeed fixed the assertion errors we were seeing.

Great, thanks for checking.

FWIW, just using the default_rng() also worked. Is it functionally any different than this?

I want to use a separate RNG stream from the one used by the rand(elt, dims)/randn(elt, dims) array constructors, since it could cause Index ID clashes if someone sets the seed of default_rng() with Random.seed!(seed) (that's particularly an issue with saving and loading ITensors). It isn't clear to me that I can "copy" the default_rng() for that purpose, but maybe I'm missing something.

@mtfishman mtfishman merged commit 323b58f into main Apr 15, 2025
15 checks passed
@mtfishman mtfishman deleted the index_rng branch April 15, 2025 22:43
@amilsted
Copy link

Btw, credit to @wildart for finding the issue and testing the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants